Add `get_size_on_disk` method to `RemoteData` #6584

GeigerJ2 · 2024-10-15T16:00:05Z

Required for PR #6578.

By default, the get_size_on_disk method calls the method get_size_on_disk_du that uses du to obtain the total directory size in bytes. The output will eventually be formatted in a human-readable way. If the call to du fails for whatever reason, recursive lstat is being used, though, that is discouraged due to (copied from _get_size_on_disk_lstat docstring):

"Note that even if a file is only 1 byte, on disk, it still occupies one full disk block size. As such, getting accurate measures of the total expected size on disk when retrieving a RemoteData is not straightforward with lstat, as one would need to consider the occupied block sizes for each file, as well as repository metadata. Thus, this function only serves as a fallback in the absence of the du command."

I further extended the existing tests for RemoteData to use both, LocalTransport, as well as SshTransport, and test for the functionality added in this PR.

Pinging also @npaulish, as she's currently working on retrieving RemoteData objects.

codecov · 2024-10-15T16:32:14Z

Codecov Report

Attention: Patch coverage is 80.00000% with 11 lines in your changes missing coverage. Please review.

Project coverage is 77.91%. Comparing base (ef60b66) to head (ceb0673).
Report is 136 commits behind head on main.

Files with missing lines	Patch %	Lines
src/aiida/orm/nodes/data/remote/base.py	77.78%	10 Missing ⚠️
src/aiida/common/utils.py	90.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6584      +/-   ##
==========================================
+ Coverage   77.51%   77.91%   +0.41%     
==========================================
  Files         560      567       +7     
  Lines       41444    42200     +756     
==========================================
+ Hits        32120    32875     +755     
- Misses       9324     9325       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

giovannipizzi · 2024-10-17T12:14:19Z

This is maybe machine-dependent, but rather than going via our API (that is more robust, but definitely going to be slower, I think) have a first "fast" option just running du -s and parsing the output (but careful about units! E.g. it uses "blocks", on some systems it's 512 or 2048bytes!! And if it fails, fall back to your solution?

GeigerJ2 · 2024-10-17T12:31:05Z

Note to self to run du via exec_command_wait method from transport.

khsrali

Thanks @GeigerJ2 please go through more pains that I imposed 😈

src/aiida/common/utils.py

src/aiida/orm/nodes/data/remote/base.py

khsrali · 2024-11-21T15:31:32Z

src/aiida/orm/nodes/data/remote/base.py

+        :rtype: int
+        """
+
+        retval, stdout, stderr = transport.exec_command_wait(f'du --bytes {full_path}')


Suggested change

retval, stdout, stderr = transport.exec_command_wait(f'du --bytes {full_path}')

retval, stdout, stderr = transport.exec_command_wait(f'du -h {full_path}')

that already returns the human readable one, why not?

At least for testing and validation, I think returning the bytes is more convenient. I'd convert it to the human-readable format only at the last step, when printing it to the user.

khsrali · 2024-11-21T15:37:28Z

src/aiida/orm/nodes/data/remote/base.py

+            Helper function for recursive directory traversal to obtain the `listdir_withattributes` result for all
+            subdirectories.
+
+            :param path: Path to be traversed.


absolute or relative?

Let's make it absolute then ;)

khsrali · 2024-11-21T15:39:11Z

src/aiida/orm/nodes/data/remote/base.py

+            for item in contents:
+                item_path = os.path.join(path, item['name'])
+                if item['isdir']:
+                    # Include size of direcotry


I'm confused here, is the size of a directory different from it's contents?

Yes, the size returned from this call is the size of the directory inode, which includes the metadata of its (file) contents, but not the actual, total (file) content size. Very annoying indeed.

src/aiida/orm/nodes/data/remote/base.py

khsrali · 2024-11-21T15:50:14Z

src/aiida/orm/nodes/data/remote/base.py

+
+        return format_directory_size(size_in_bytes=total_size)
+
+    def _get_size_on_disk_du(self, full_path: Path, transport: 'Transport') -> int:


this whole function, doesn't really need to be separated I feel.. It's basically only executes a command, and you don't re-use it, right?

I found the design to have just one top-level method get_size_on_disk nice, which tries to call the two private methods, in terms of separation of concerns.

Though, it is true that this pollutes the API of RemoteData somewhat... So I'm also fine of either moving the private methods to some utils module, or merging them. Maybe @unkcpz can comment on good coding practices here?

which tries to call the two private methods, in terms of separation of concerns.

I'm not sure if I understand, sorry 😬
Since it's not really re-usable by other method, I'd vote for merging it, and avoid over-fictionalizing

khsrali · 2024-11-21T15:56:25Z

src/aiida/orm/nodes/data/remote/base.py

+        :rtype: int
+        """
+
+        retval, stdout, stderr = transport.exec_command_wait(f'du --bytes {full_path}')


one more concern, is the aiida-firecrest plugin..
they don't support command execution, so it's better we avoid adding more calls to exec_command_wait in the code base, wherever it's not absolutely crucial...
I mean it adds maintenance overheads.. in future somebody will open an issue and PR to change this..

Also your nice _get_size_on_disk_lstat function is already addressing this functionality, so du doesn't seem super crucial

Hm, I can add a check on the transport-type before, to make sure it stays compatible with FirecREST in the future. Though, I wouldn't remove the convenient and preferred implementation for now in anticipation of FirecREST eventually becoming the required transport mechanism for CSCS.

yes, please! Makes sense to add a except NotImplementedError on line 232, in case exec_command_wait is not implemented.

khsrali · 2024-11-21T16:02:44Z

tests/orm/nodes/data/test_remote.py

+@pytest.fixture
+def remote_data_ssh(tmp_path, aiida_computer_ssh):
+    """Return a non-empty ``RemoteData`` instance."""
+    # Compared to `aiida_localhost`, `aiida_computer_ssh` doesn't return an actual `Computer`, but just a factory


🙉 aiida_computer_ssh should be compared with aiida_computer_local, which in that sense they are similar.. the issue is we don't have "aiida_ssh" that would return the actual computer instance, lol 🙉

Yeah, exactly. We could add a fixture that actually calls the factory and returns the Computer instance, though I'd rather call it aiida_localhost_ssh, and don't see the need for it right now.

yews, so maybe please fix this comment, which I found confusing:

Suggested change

# Compared to `aiida_localhost`, `aiida_computer_ssh` doesn't return an actual `Computer`, but just a factory

# `aiida_computer_ssh` doesn't return an actual `Computer`, but just a factory

GeigerJ2

Not sure why my replies via the files GH tab appear as a review, but whatever... ^^

Thanks for the review @khsrali. I implemented most of your proposed changes!

As to our in-person discussion if du or lstat is preferred, I think none of the two is ideal... lstat giving the actual byte-sized content, which is neat and all, but won't correspond to the disk space that will actually be occupied locally by a file, due to the use of blocks for the file system. And du giving the actual occupied disk space on the remote, which, however, might be different from the local file system (due to having a different file system on the local machine, different formatting, different block size, etc.). Hence, the big difference in the file size check in the test. For real-world use cases, with more and larger files, the difference is likely much smaller, and won't matter too much, I think. I'll add a test for a larger file, as well as modify the message that the given size is just an estimate, so that user are aware they should take the value with a grain of salt.

Maybe also @agoscinski with his actual computer science background can weigh in 🫶

src/aiida/orm/nodes/data/remote/base.py

GeigerJ2 · 2024-11-25T10:57:42Z

src/aiida/orm/nodes/data/remote/base.py

+
+        return format_directory_size(size_in_bytes=total_size)
+
+    def _get_size_on_disk_du(self, full_path: Path, transport: 'Transport') -> int:


I found the design to have just one top-level method get_size_on_disk nice, which tries to call the two private methods, in terms of separation of concerns.

Though, it is true that this pollutes the API of RemoteData somewhat... So I'm also fine of either moving the private methods to some utils module, or merging them. Maybe @unkcpz can comment on good coding practices here?

GeigerJ2 · 2024-11-25T10:58:30Z

src/aiida/orm/nodes/data/remote/base.py

+        :rtype: int
+        """
+
+        retval, stdout, stderr = transport.exec_command_wait(f'du --bytes {full_path}')


At least for testing and validation, I think returning the bytes is more convenient. I'd convert it to the human-readable format only at the last step, when printing it to the user.

GeigerJ2 · 2024-11-25T12:04:09Z

src/aiida/orm/nodes/data/remote/base.py

+        :rtype: int
+        """
+
+        retval, stdout, stderr = transport.exec_command_wait(f'du --bytes {full_path}')


Hm, I can add a check on the transport-type before, to make sure it stays compatible with FirecREST in the future. Though, I wouldn't remove the convenient and preferred implementation for now in anticipation of FirecREST eventually becoming the required transport mechanism for CSCS.

GeigerJ2 · 2024-11-25T12:08:12Z

src/aiida/orm/nodes/data/remote/base.py

+            for item in contents:
+                item_path = os.path.join(path, item['name'])
+                if item['isdir']:
+                    # Include size of direcotry


Yes, the size returned from this call is the size of the directory inode, which includes the metadata of its (file) contents, but not the actual, total (file) content size. Very annoying indeed.

src/aiida/orm/nodes/data/remote/base.py

GeigerJ2 · 2024-11-25T12:13:34Z

src/aiida/orm/nodes/data/remote/base.py

+            Helper function for recursive directory traversal to obtain the `listdir_withattributes` result for all
+            subdirectories.
+
+            :param path: Path to be traversed.


Let's make it absolute then ;)

In addition, extend the tests for `RemoteData` in `test_remote.py` for the methods added in this PR, as well as parametrize them to run on a `RemoteData` via local and ssh transport.

for more information, see https://pre-commit.ci

khsrali

Thanks a lot @GeigerJ2! Just a few minor comments..

src/aiida/common/utils.py

khsrali · 2024-11-25T15:34:25Z

src/aiida/orm/nodes/data/remote/base.py

+        :rtype: int
+        """
+
+        retval, stdout, stderr = transport.exec_command_wait(f'du --bytes {full_path}')


yes, please! Makes sense to add a except NotImplementedError on line 232, in case exec_command_wait is not implemented.

khsrali · 2024-11-25T15:37:04Z

tests/orm/nodes/data/test_remote.py

+@pytest.fixture
+def remote_data_ssh(tmp_path, aiida_computer_ssh):
+    """Return a non-empty ``RemoteData`` instance."""
+    # Compared to `aiida_localhost`, `aiida_computer_ssh` doesn't return an actual `Computer`, but just a factory


yews, so maybe please fix this comment, which I found confusing:

Suggested change

# Compared to `aiida_localhost`, `aiida_computer_ssh` doesn't return an actual `Computer`, but just a factory

# `aiida_computer_ssh` doesn't return an actual `Computer`, but just a factory

khsrali · 2024-11-25T15:43:54Z

src/aiida/orm/nodes/data/remote/base.py

+        return format_directory_size(size_in_bytes=total_size)
+
+    def _get_size_on_disk_du(self, full_path: Path, transport: 'Transport') -> int:
+        """Connects to the remote folder and returns the total size of all files in the directory recursively in bytes


Suggested change

"""Connects to the remote folder and returns the total size of all files in the directory recursively in bytes

"""Connects to the remote folder and returns the total size of all files in the directory in bytes

khsrali · 2024-11-25T15:45:32Z

src/aiida/orm/nodes/data/remote/base.py

+        :param transport: Open transport instance
+        :type transport: Transport
+        :raises RuntimeError: When `du` command cannot be successfully executed
+        :return: Total size of directory recursively in bytes.


Sorry, I found the term recursively confusing.. (thought the function does yield )

Suggested change

:return: Total size of directory recursively in bytes.

:return: Total size of directory in bytes (including all it's contents)

khsrali · 2024-11-25T15:48:50Z

src/aiida/orm/nodes/data/remote/base.py

+        :param transport: Open transport instance.
+        :type transport: Transport
+        :raises RuntimeError: When `du` command cannot be successfully executed.
+        :return: Total size of directory recursively in bytes.


the same:

Suggested change

:return: Total size of directory recursively in bytes.

:return: Total size of directory in bytes (including all it's contents)

khsrali · 2024-11-25T15:56:13Z

src/aiida/orm/nodes/data/remote/base.py

+
+        return format_directory_size(size_in_bytes=total_size)
+
+    def _get_size_on_disk_du(self, full_path: Path, transport: 'Transport') -> int:


which tries to call the two private methods, in terms of separation of concerns.

I'm not sure if I understand, sorry 😬
Since it's not really re-usable by other method, I'd vote for merging it, and avoid over-fictionalizing

GeigerJ2 marked this pull request as draft October 15, 2024 16:02

GeigerJ2 mentioned this pull request Oct 15, 2024

Add --also-remote option to verdi process dump #6578

Open

GeigerJ2 changed the title ~~Add get_total_size_on_disk method to RemoteData.~~ Add get_total_size_on_disk method to RemoteData Oct 17, 2024

mikibonacci mentioned this pull request Oct 17, 2024

On clean remote data button and checks aiidalab/aiidalab-qe#857

Open

3 tasks

GeigerJ2 requested a review from khsrali November 21, 2024 14:53

GeigerJ2 force-pushed the feature/remote-data-total-size branch from e801a78 to e0be575 Compare November 21, 2024 14:54

GeigerJ2 marked this pull request as ready for review November 21, 2024 14:54

GeigerJ2 changed the title ~~Add get_total_size_on_disk method to RemoteData~~ Add get_size_on_disk method to RemoteData Nov 21, 2024

GeigerJ2 force-pushed the feature/remote-data-total-size branch from e0be575 to b1a0560 Compare November 21, 2024 15:14

khsrali requested changes Nov 21, 2024

View reviewed changes

GeigerJ2 commented Nov 25, 2024

View reviewed changes

GeigerJ2 and others added 6 commits November 25, 2024 16:41

Add get_total_size_on_disk method to RemoteData.

1aeea2f

Implement function with du and lstat fallback

d7af702

In addition, extend the tests for `RemoteData` in `test_remote.py` for the methods added in this PR, as well as parametrize them to run on a `RemoteData` via local and ssh transport.

Implement function with du and lstat fallback

10e0a62

In addition, extend the tests for `RemoteData` in `test_remote.py` for the methods added in this PR, as well as parametrize them to run on a `RemoteData` via local and ssh transport.

Apply changes from code review by @khsrali

d08be75

[pre-commit.ci] auto fixes from pre-commit.com hooks

26abd5a

for more information, see https://pre-commit.ci

Add tests for different file sizes.

4fcb1ba

GeigerJ2 force-pushed the feature/remote-data-total-size branch from f2f90f9 to 4fcb1ba Compare November 25, 2024 15:41

khsrali requested changes Nov 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `get_size_on_disk` method to `RemoteData` #6584

Add `get_size_on_disk` method to `RemoteData` #6584

GeigerJ2 commented Oct 15, 2024 •

edited

Loading

codecov bot commented Oct 15, 2024 •

edited

Loading

giovannipizzi commented Oct 17, 2024

GeigerJ2 commented Oct 17, 2024 •

edited

Loading

khsrali left a comment

khsrali Nov 21, 2024

GeigerJ2 Nov 25, 2024

khsrali Nov 21, 2024

GeigerJ2 Nov 25, 2024

khsrali Nov 21, 2024

GeigerJ2 Nov 25, 2024

khsrali Nov 21, 2024

GeigerJ2 Nov 25, 2024

khsrali Nov 25, 2024

khsrali Nov 21, 2024

GeigerJ2 Nov 25, 2024

khsrali Nov 25, 2024

khsrali Nov 21, 2024

GeigerJ2 Nov 25, 2024

khsrali Nov 25, 2024

GeigerJ2 left a comment

GeigerJ2 Nov 25, 2024

GeigerJ2 Nov 25, 2024

GeigerJ2 Nov 25, 2024

GeigerJ2 Nov 25, 2024

GeigerJ2 Nov 25, 2024

khsrali left a comment

khsrali Nov 25, 2024

khsrali Nov 25, 2024

khsrali Nov 25, 2024

khsrali Nov 25, 2024

khsrali Nov 25, 2024

khsrali Nov 25, 2024

	retval, stdout, stderr = transport.exec_command_wait(f'du --bytes {full_path}')
	retval, stdout, stderr = transport.exec_command_wait(f'du -h {full_path}')


		return format_directory_size(size_in_bytes=total_size)

		def _get_size_on_disk_du(self, full_path: Path, transport: 'Transport') -> int:

	# Compared to `aiida_localhost`, `aiida_computer_ssh` doesn't return an actual `Computer`, but just a factory
	# `aiida_computer_ssh` doesn't return an actual `Computer`, but just a factory

	"""Connects to the remote folder and returns the total size of all files in the directory recursively in bytes
	"""Connects to the remote folder and returns the total size of all files in the directory in bytes

	:return: Total size of directory recursively in bytes.
	:return: Total size of directory in bytes (including all it's contents)

Add get_size_on_disk method to RemoteData #6584

Are you sure you want to change the base?

Add get_size_on_disk method to RemoteData #6584

Conversation

GeigerJ2 commented Oct 15, 2024 • edited Loading

codecov bot commented Oct 15, 2024 • edited Loading

Codecov Report

giovannipizzi commented Oct 17, 2024

GeigerJ2 commented Oct 17, 2024 • edited Loading

khsrali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeigerJ2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khsrali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `get_size_on_disk` method to `RemoteData` #6584

Add `get_size_on_disk` method to `RemoteData` #6584

GeigerJ2 commented Oct 15, 2024 •

edited

Loading

codecov bot commented Oct 15, 2024 •

edited

Loading

GeigerJ2 commented Oct 17, 2024 •

edited

Loading